I added qartod.py inside the esdglider folder, and I added qartod-config.yml in esdglider/data. Also, I messed up my branch name because I've been working with Quarto a lot, so the branch should've been qartod-module, but instead I accidentally named it quarto-module.

Hi Madi - thanks for this! I've put some comments inline. A couple general ones, happy to discuss any of these:

For log messages, please use the syntax _log.info("QCing %s", var), rather than f-strings. I haven't run it in a while, but this comes from the pre-commit hooks that I (intermittently) use. I definitely need to pick a specific style guide, and have CoPilot go through and make this consistent :)

For me, having extra blank lines, eg between function calls makes the code less easily readable (example below). For esdglider, I'd like to stick to the principles of a) generally keeping code on one line is the line length is <80, and b) not having extra blank lines between lines in the same function.

# Please change 
final_flags = np.maximum.reduce(

    test_results

).astype("int8")

# to either:
final_flags = np.maximum.reduce(test_results).astype("int8")

# or:
final_flags = np.maximum.reduce(
    test_results
).astype("int8")

Hi Sam,

I'll make sure to fix all of these!

Hi Madi - thanks for this! I've put some comments inline. A couple general ones, happy to discuss any of these:

For log messages, please use the syntax _log.info("QCing %s", var), rather than f-strings. I haven't run it in a while, but this comes from the pre-commit hooks that I (intermittently) use. I definitely need to pick a specific style guide, and have CoPilot go through and make this consistent :)

For me, having extra blank lines, eg between function calls makes the code less easily readable (example below). For esdglider, I'd like to stick to the principles of a) generally keeping code on one line is the line length is <80, and b) not having extra blank lines between lines in the same function.

# Please change 
final_flags = np.maximum.reduce(

    test_results

).astype("int8")

# to either:
final_flags = np.maximum.reduce(test_results).astype("int8")

# or:
final_flags = np.maximum.reduce(
    test_results
).astype("int8")

Do you want me to remove the comments in the code (example below)?

EVALUATE ALL DATA VARIABLES

for var in ds.data_vars:
    # REQUIRE TIME DIMENSION
    if "time" not in ds[var].dims:
        continue
    # SKIP EXISTING QC VARIABLES
    if var.endswith("_qc"):
        continue
    # SKIP EXCLUDED VARIABLES
    #
    # Exclude metadata variables and redundant
    # coordinate variables that should not
    # receive QARTOD testing.
    if var in skip_variables:
        continue
    time_variables.append(var)
# LOG SUMMARY
_log.info(
    "Found %d time variables for QC",
    len(time_variables)
)
return time_variables

No, I think the comments are overall good, although I typically don't like them over logs because (unless it's really complicated) I think the log should be self-explanatory. I would format that block something like:

for var in ds.data_vars:
    # REQUIRE TIME DIMENSION
    if "time" not in ds[var].dims:
        continue

    # SKIP EXISTING QC VARIABLES AND EXCLUDED VARIABLES
    # {SMW note: I think fine to add more context here, if you feel it's necessary}
    if var.endswith("_qc") or var in skip_variables:
        continue

    time_variables.append(var)

_log.info(
    "Found %d time variables for QC",
    len(time_variables)
)

return time_variables

Thanks!

I updated everything based on your comments. If there's anything else, please let me know!

Thank you! This is looking good. I'll try out the fork tomorrow.

One other ask - could you add a notebook (to the 'notebooks' folder) that shows a representative example or two of using this functionality?

Yes! Sorry to get back late, I have been traveling. I'll make sure to add a notebook today.

Thanks, this is looking good. The outputs make sense

A couple more requests to complete the pr are below; sorry I didn't think to include these initially.

• Add 'ioos_qc' to the pyproject.toml and environment.yml files; it can go into the pyproject file as a normal dependency.
• Add yourself as an author in the pyproject.toml file
• Add a blurb to the CHANGLOG file. For instance, under the 'Module and function restructuring' heading: "Added a qartod module, for generating qartod flags using the ioos_qc package for the science dataset". Note: to make this change, you'll need to update your branch with the updates that I've pushed to esdglider:main. If that is a hassle, no worries, and I can add this line after merging in the PR.
• In the paths module, can you add a function (maybe get_path_qartod_config) for getting the path to the qartod-config.yml file? A la get_path_yaml. Sidebar, I need to rename the get_path_yaml function to differentiate it, probably to get_path_yaml_deployment_var. I'll do that in the main branch shortly.
• It would be quite useful to have a way of visualizing/summarizing the generated QC flags, that could be easy to run for every deployment. I'm thinking of timeseries plots of each QC variable, which could be added to the plots folder. Another options could be a numerical summary printed to the logs, eg of the number of suspect/fail flags per variable. But maybe you/Dale/Cara have a better idea for this?

I updated the requests you gave, but for visualizing/logging of the qc flags, what would be most helpful for you? Are you interested in seeing exactly when the bad flags occur or just generally how many flags (of each kind) there are?

Thanks @madrichardson! Updates look great. For visualizing/summarizing, I'm thinking both a histogram (or logged summary counts) of the number of each flag per variable, and then a timeseries of the flags. For the timeseries, either using color-coding to mark 'bad' flags, or have the plot only be of the 'bad' flags.

Hi Sam, the Glider DAC folks provided guidance for determining the thresholds for two of the tests for qc'ing. They already have functions for both: get_spike_thresholds() & get_rate_of_change_threshold(). Did you want me to incorporate these in the qartod module? I was thinking I would update qartod.py with these and then edit parts of the script so that if would load the qartod-config.yml, create the config-dictionary, compute the threshold statistics, replace the placeholder thresholds, and then update the config-dictionary so that the qartod-config.yml is never overwritten, just used as a template.

Also, I saw you were out of office until August, so no worries about getting back to me until then!

Thanks Madi. This all makes sense to me.

I like just updating the config dictionary, rather than having to update qartod-config.yml all the time. When adding functions, please just make sure to provide credit/links in the function docstring that points back to the DAC's original function

Also, maybe this happens by default, but will the output nc file have these function-calculated values as an attribute for the applicable quartod variable? Eg, will qartod_density_spike_flag have an attribute that contains the calculated spike thresholds? I think that could be useful for any future troubleshooting

Hi Sam,

Sorry for going MIA, I've been busy with some other projects. The output nc file wouldn't have the function calculated values as an attribute for the applicable quartod variable, but I agree that it would be useful to include. I'm thinking that we could store the threshold calculations in a dictionary and then add it as an argument for the create_qc_variables() function.

I'm going to try to get the functions implemented by the end of the week, and I'll be working on the qc flag figures as well.

First off, sorry for the commit - I thought I was just pulling changes, but I think a couple of VS Code extensions crossed paths, and so thought they had to merge the PR with the branch for some reason. I don't think my 'commit' actually changes anything (unless it pulled updates from main), but if it did let me know and I'll happily revert.

No worries @madrichardson - this sound good. I'll be around the next couple of weeks, so should be able to respond to things quickly.

From some quick browsing, I like how this dataset uses a flag_configuration attribute to include the different threshold values for the different qartod tests. However, as long as it's reproducible and let's us trace back to the threshold values some way for reference, I'd say go ahead and go with what makes sense to you since you've been in this.

I added the threshold functions and now I've created a flag_configuration attribute for each variable that looks like this:
Example is from temperature
{'configuration_source': 'template_modified_in_memory',
'configuration_template': 'qartod-config.yml',
'flat_line_test': {'fail_threshold': 5000,
'suspect_threshold': 3000,
'tolerance': 0.001},
'gross_range_test': {'fail_span': [-2, 40], 'suspect_span': [0, 35]},
'rate_of_change_test': {'threshold': 1.303997039794922e-10},
'spike_test': {'fail_threshold': 4.362772510393401,
'suspect_threshold': 2.1813862551967005},
'threshold_source': 'computed_from_deployment_statistics'}

What do you think?

From some quick browsing, I like how this dataset...

Oops - I meant to link this dataset

What do you think?

That looks awesome! Thanks for wrangling all of that together into that attribute

Hi Sam,

I've been working on the plots for visualizing the qc flags and I wanted your opinion. I've been using a stacked bar chart for the qc_flag quantities so you can have all the qc variables on one plot instead of a plot for each variable.

I've been using a stacked bar chart for the qc_flag quantities so you can have all the qc variables on one plot instead of a plot for each variable.

@madrichardson these look great - I also like seeing them like that. Thinking out loud, I think a plot like this would be useful for the whole deployment. For deployments with 100s of profiles, it would be hard to scan through 100s of plots - perhaps a CSV file with one column for each flag value, and one row for each profile?

I think the (only?) other plot we'll need is one for each variable of the 'timeseries' of the QC flags, like the plots in the 'timeSeries-sci' plot folder.
If I haven't sent you those plots, you should be able to see examples here

So are you saying one plot for the entire deployment where for every QC NetCDF in the deployment, accumulate the flag counts for each variable? And then have the CSV file with one column for each flag value, and one row for each profile?

Yep. But happy if you have other suggestions!

I think regardless though, the plots function is still going to be the same: a function that takes in an xarray dataset, and does a bar plot of all the _qc variables in that dataset? And then we can either pass that function each profile, or a full deployment.

I think that's a good idea! Where do you want me to put the plotting function? In plots.py, in qartod.py, or another location?

In plots.py would be great

Sorry I keep pestering you with questions, but do you want me to call the plotting function in qartod.py or is that something you'll call somewhere else in the entire processing pipeline?

Not worries at all. I'm doing other glider coding things, so this is a perfect time.

For this PR, let's just add the plotting functions, and an example of using them to the notebook.

I may want them in plots.esd_all_plots in the future, but let's hold for now until we can see how they feel when we add all the QC pieces to the standard processing scripts.

Sounds good to me, thanks!

Here is the full deployment stacked bar chart plot, the csv file with the flag quantities, and a couple examples of the timeseries plots. Let me know what you think, and if you like them I can push my updates to the PR.

amlr08-20220513-deployment_qc_summary.csv

Here's some others with more diverse flags:

These look great Madi! Thank you! My only request is to add the actual profile index (in addition to the profile name) to the CSV.

It's curious that depth has so many suspect flags. I wonder why that is, and if it will be similar for other deployments.

Here's some others with more diverse flags:

We got a notice from the manufacturer that the cdom data for this deployment is irretrievable. So, it's good that the CDOM data looks so bad!